Fix concurrent-extension race in MMapDirectory.Map, #1090 - #1263
Merged
Conversation
When a file is being appended to concurrently (e.g. by an IndexWriter
that still holds a write handle), MemoryMappedFile.CreateFromFile can
throw ArgumentOutOfRangeException("capacity") because its internal stat
observes a file size greater than the capacity we computed from
fc.Length moments earlier. Take the max of the caller-supplied length
and a fresh fc.Length read as the capacity, and retry with an updated
length on that specific failure. Adds test-only counters and a
stress-based regression test in TestMultiMMap.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a concurrency race in Lucene.Net.Store.MMapDirectory.Map() where a file growing between reading fc.Length and MemoryMappedFile.CreateFromFile()’s internal size check could throw ArgumentOutOfRangeException("capacity") (issue #1090).
Changes:
- Adds a capacity selection + retry loop around
MemoryMappedFile.CreateFromFile()to handle concurrent file extension. - Introduces internal counters to observe retry behavior (used by tests).
- Adds a slow stress/regression test that extends a file concurrently while repeatedly calling
MMapDirectory.OpenInput.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Lucene.Net/Store/MMapDirectory.cs |
Adds retry logic for CreateFromFile() capacity race and internal counters for observability. |
src/Lucene.Net.Tests/Store/TestMultiMMap.cs |
Adds a slow regression test that attempts to reproduce the concurrent-extension race from #1090. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Narrow the ArgumentOutOfRangeException retry filter to ParamName == "capacity" so unrelated argument errors aren't masked, and mark the regression test NonParallelizable since it relies on static counters on MMapDirectory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a concurrent race bug in MMapDirectory.Map.
Fixes #1090
Description
When a file is being appended to concurrently (e.g. by an
IndexWriterthat still holds a write handle, as inTestNeverDelete),MemoryMappedFile.CreateFromFilecan throwArgumentOutOfRangeException("capacity")with the messageThe capacity may not be smaller than the file size.This happens because the BCL performs an internal stat and compares the current file size against the capacity we passed — which was computed fromfc.Lengthmoments earlier. If the file grows in between, the check fails.The fix, in
MMapDirectory.Map:lengthand a freshfc.Lengthread as the initial capacity.ArgumentOutOfRangeException(up to 5 attempts) with the latest observed length, since the window between our stat and the BCL's internal stat still allows further growth.The buffer-sizing loop continues to use the caller-supplied
length, so an oversized capacity is harmless — we never expose the extra bytes.Testing
Added
TestMultiMMap.TestOpenInputConcurrentFileExtension_Issue1090, a stress-based regression test that spins a background thread extending a small file (capped at 1 MiB) while the foreground repeatedly callsMMapDirectory.OpenInput. On unpatched code, this reproduces theexact stack trace from #1090 within milliseconds. With the fix, the test passes.
To avoid the "silently passes because the race didn't fire" trap, two internal counters (
s_capacityRetryCount,s_maxCapacityAttemptsObserved) record retry activity, and the test:Observed on macOS/ARM across 5 runs: race triggered within 4–120 iterations; 1 retry always sufficed (
maxAttemptsObserved = 2). The configured max of 5 is defensive padding.Test is marked
[Slow].AI: Generated with Claude Code Opus 4.7.